Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Update snapshot and record #742

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

skoudoro
Copy link
Contributor

@skoudoro skoudoro commented Feb 8, 2023

This PR update snapshot and record function to allow ShowManager as input argument and deprecate Scene as an input arguments.

Many PR will follow-up to clean our tests and tutorial by forcing the use of ShowManager

#196

@Garyfallidis
Copy link
Contributor

It is not just that right? It should be also about resolving some memory management issues.

@Garyfallidis
Copy link
Contributor

Or this is for the next PR?

@skoudoro
Copy link
Contributor Author

skoudoro commented Feb 8, 2023

Yes and No @Garyfallidis.

Yes -> it promotes good practice and avoid the memory leak. see below, no memory leak

    t = np.linspace(-10, 10, 100)
    bundle = []
    for i in np.linspace(3, 5, 1000):
        pts = np.vstack((np.cos(2 * t/np.pi), np.zeros(t.shape) + i*10, t )).T
        bundle.append(pts)

   # No memory when show manager outside the loop
    showm = window.ShowManager()
    for time in range(200):   
        
        ren.scene.clear()
        bundle_actor = actor.streamtube(bundle, window.colors.red, linewidth=0.01)
        showm.scene.add(bundle_actor)
        showm.scene.SetBackground(*window.colors.white)
        image_array = window.snapshot(
            showm, fname='images/file%02d.png' % time, size=(1000, 1000))

No -> the memory leak is from VTK, C++ based, so it will be always there until they fix it. the example below have a memory leak:

    t = np.linspace(-10, 10, 100)
    bundle = []
    for i in np.linspace(3, 5, 1000):
        pts = np.vstack((np.cos(2 * t/np.pi), np.zeros(t.shape) + i*10, t )).T
        bundle.append(pts)

    # ren = window.Scene()
    for time in range(200):  
        # Memory leak here, creation of multiple RenderWindow not well freed by vtk 
        ren = window.ShowManager()
        ren.scene.clear()
        bundle_actor = actor.streamtube(bundle, window.colors.red, linewidth=0.01)
        ren.scene.add(bundle_actor)
        ren.scene.SetBackground(*window.colors.white)
        image_array = window.snapshot(
            ren, fname='images/file%02d.png' % time, size=(1000, 1000))

@skoudoro
Copy link
Contributor Author

This PR is ready to go @Garyfallidis.

2 PR will follow this PR: Update tests, then update tutorials

Copy link
Contributor

@Garyfallidis Garyfallidis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many lines spent. Otherwise good.


self.window.SetOffScreenRendering(previous_offscreen)

def snapshot(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the next PR please bring these parameters up so we can save a few lines. It makes reading the code much easier.

@Garyfallidis
Copy link
Contributor

Unfortunately, there are some issues with viz_timer.py in Ubuntu. Happy to zoom. Hard to explain with words here.

@Garyfallidis Garyfallidis changed the title Update snapshot and record WIP: Update snapshot and record Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants